Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error message when using an invalid object key #1193

Closed
wants to merge 1 commit into from
Closed

Conversation

BenderScript
Copy link
Contributor

Fixes #516

  • Changed the error string to include the immediate outer object, key
    type and value.
  • Fix the shadowing of variable v by the function called by Iter so
    object can be printed.

Signed-off-by: repenno rapenno@gmail.com

Fixes #516

* Changed the error string to include the immediate outer object, key
type and value.
* Fix the shadowing of variable v by the function called by Iter so
object can be printed.

Signed-off-by: repenno <rapenno@gmail.com>
Copy link
Member

@tsandall tsandall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may improve debuggability a bit, however, there are two things that come to mind...

  1. Including values computed during evaluation inside of error messages can be risky. We don't want secret values operated on inside of the policy to accidentally leak out into error messages or logs.

  2. Errors generated deep into the call stack will still be hard to locate.

I think what I'd rather see here is an improvement to the evaluator to return errors with the source location where the error occurred (e.g., the expression location.)

@BenderScript
Copy link
Contributor Author

Your comment changes the scope of #516 considerably. We can go with this fix for now or put #516 back in the pile with your added comments. Or maybe close #516 and add something new. Let me know.

@tsandall
Copy link
Member

tsandall commented Feb 4, 2019

I don't know if the scope is increased considerably, the change is likely localized to the place where the error is generated in eval.go but we'd need to investigate first.

If you want to just link to my comment in the above issue, that would be fine. It would be better not to merge this because of the two issues mentioned above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants